[EventHub][ServiceBus] Fix pyAMQP decode of performatives with omitted trailing null fields#47660
Open
j7nw4r wants to merge 7 commits into
Open
[EventHub][ServiceBus] Fix pyAMQP decode of performatives with omitted trailing null fields#47660j7nw4r wants to merge 7 commits into
j7nw4r wants to merge 7 commits into
Conversation
j7nw4r
marked this pull request as ready for review
July 15, 2026 22:56
j7nw4r
requested review from
EldertGrootenboer,
SwayGom,
axisc,
hmlam,
sagar0207,
sjkwak and
skarri-microsoft
as code owners
July 15, 2026 22:56
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes pyAMQP decoding of performatives with omitted trailing fields across Event Hubs and Service Bus.
Changes:
- Pads shortened performatives and supports
list0. - Adds regression tests for Open and Transfer frames.
- Updates changelogs and Event Hubs versioning.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/servicebus/azure-servicebus/tests/unittests/test_pyamqp_decode.py |
Adds decoder regression tests. |
sdk/servicebus/azure-servicebus/CHANGELOG.md |
Documents the fix. |
sdk/servicebus/azure-servicebus/azure/servicebus/_pyamqp/_decode.py |
Implements performative padding and list0 decoding. |
sdk/eventhub/azure-eventhub/tests/pyamqp_tests/unittest/test_decode.py |
Adds equivalent regression tests. |
sdk/eventhub/azure-eventhub/CHANGELOG.md |
Adds the 5.15.2 release entry. |
sdk/eventhub/azure-eventhub/azure/eventhub/_version.py |
Bumps version to 5.15.2. |
sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_decode.py |
Mirrors the decoder fix. |
j7nw4r
force-pushed
the
fix/pyamqp-decode-short-list
branch
from
July 16, 2026 18:19
6fecb8a to
f89b410
Compare
AMQP 1.0 section 1.4 lets a sender omit trailing null fields, so an incoming performative described-list can be shorter than the full field count. The pyAMQP decoder built the field list from the wire count, and consumers then accessed fixed indices (frame[9], OpenFrame(*frame)), raising IndexError/TypeError on a short list. decode_frame now pads the decoded list up to the performative's full field count, so omitted trailing fields read back as None. Applied to both the Event Hubs and Service Bus vendored copies, with regression tests and changelog entries.
…ives Add the # type: ignore # pylint: disable=protected-access suppressions on the _PERFORMATIVE_FIELD_COUNT comprehension, matching every other access to the protected _code/_definition attributes in this engine (see _encode.py), so the pylint and mypy CI gates stay green. Also handle the AMQP list0 (0x45) body encoding in decode_frame: a performative whose fields are all omitted may arrive as list0, which carries no count byte. Previously this read data[5] out of range and raised IndexError before the padding ran. It is now treated as zero fields and padded to the full field count, closing the "omitted trailing null fields" case for End and Close. Add list0 regression tests to both the eventhub and servicebus suites.
…drift Extend the decode regression tests for the trailing-null-omission fix: - Parametrized short-list8 decode over Begin, Attach, Disposition, and Detach, the no-default namedtuples that raised TypeError on a short unpack pre-fix. Only Open was previously covered. - Short performative encoded as a list32 (0xd0) body, exercising the count/offset branch that no existing test reached (all fixtures used list8). - Short SASLInit and SASLOutcome frames, which also have required fields. - A skip-guarded test asserting the eventhub and servicebus _pyamqp copies of _decode.py, _encode.py, and performatives.py stay byte-identical, so a fix applied to one copy but not the other is caught. It skips when the sibling package source is not present (the packages ship separately). Applied identically to both the eventhub and servicebus decode test suites.
Decoding a short performative padded every omitted trailing field with None. For a field whose AMQP-defined default is non-null this is wrong: a minimal Open frame legitimately omits max_frame_size (default 4294967295), and _connection._incoming_open reads it positionally and numerically (`frame[2] < 512`), which raises TypeError on None. Pad each omitted field with its field default from the performative _definition instead of None, via a _PERFORMATIVE_FIELD_DEFAULTS map; _PERFORMATIVE_FIELD_COUNT is now derived from it so the two stay in lockstep. Applied to both the Event Hubs and Service Bus copies of the vendored _pyamqp engine. Tests exercise the incoming-Open numeric path (max_frame_size/channel_max materialize to their defaults and the `< 512` comparison does not raise), and the no-default-performative and transfer tests now assert the padded tail equals each field's spec default (e.g. Disposition.batchable is False, Transfer.message_format is 0).
The _PERFORMATIVE_FIELD_DEFAULTS comprehension reads _code and _definition off the performative classes directly. Those attributes are assigned at import time in performatives.py, so pylint 4.0.4 with azure-pylint-guidelines-checker cannot resolve them statically and raised 28 E1101(no-member) errors, failing the Analyze job with exit 2. Extend the existing protected-access disable on that line to also cover no-member. Comment-only, behavior unchanged; applied to both byte-identical _pyamqp copies.
j7nw4r
force-pushed
the
fix/pyamqp-decode-short-list
branch
from
July 16, 2026 21:10
fbd200d to
50c8857
Compare
The api-consistency workflow requires a committed api.md and api.metadata.yml for every package a PR touches. azure-eventhub had neither, so the check reported "Missing required API files" once this branch touched the package. Generate both with azpysdk apistub (apiview-stub-generator 0.3.28, the version eng/apiview_reqs.txt pins), which the workflow diff-gates against a fresh regeneration. The change touches only the private _pyamqp engine, so the public API surface is unchanged; this is the package's existing public API captured for the first time. Verified the local toolchain reproduces the committed azure-servicebus api.md byte-for-byte (matching apiMdSha256), so the generated eventhub file matches what CI regenerates.
…ault Only trailing fields of a performative may be omitted, so a sender that sets a later field while wanting an earlier one's default must encode that earlier field as an explicit null. A decoded null for a field whose AMQP default is non-null therefore also means that default. Decoding now normalizes those nulls so an explicit null reads back identically to an omitted field: an Open that nulls max_frame_size comes back as 4294967295 rather than None, and _connection._incoming_open's frame[2] < 512 no longer raises TypeError. Fields whose declared default is null are left as None. Applied to both byte-identical _pyamqp copies with a regression test that nulls max_frame_size while setting channel_max after it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The pyAMQP transport crashes when it decodes an AMQP performative from a sender that omitted trailing fields or encoded a field as an explicit null. The decoder now pads the decoded field list to the full field count and applies each field's AMQP default. Positional field access and namedtuple unpacking are safe, and fields read back as their declared defaults.
Motivation
AMQP 1.0 section 1.4 permits a sender to omit the trailing fields of a performative when their values equal the defaults. An incoming performative can thus be shorter than its full field count. The decoder in
_decode.pybuilt the field list from the element count on the wire. Downstream code then reads fixed positions, for exampleframe[9]for Attachinitial_delivery_countandOpenFrame(*frame)namedtuple unpacking. On a short list this raisedIndexErrororTypeError.Some fields have a non-null default.
_incoming_opencomputesframe[2] < 512onmax_frame_size, whose default is 4294967295. If the decoder pads that field with null, the comparison raisesTypeError. The same failure occurs with an explicit null on the wire. Only trailing fields can be omitted, so a sender that sets a later field and keeps the default of an earlier field must encode the earlier field as an explicit null. The decoder must map that null back to the default.This is the receiver side of Azure/amqpnetlite#645. A spec-compliant sender that omits trailing fields makes the receiver crash. Brokers and AMQP stacks that bound decoding by the wire count are not affected.
Changes
decode_framepads the decoded performative field list to the full field count of the performative. The pad values come from each field's declared default in the performative's_definition. A field with a non-null default (for examplemax_frame_size,channel_max, or the Transfer boolean flags) reads back as that default. A field with a null default reads back asNone.decode_framereplaces a decoded explicit null with the field's declared default when that default is non-null. An explicit null now reads back the same as an omitted field. Fields with a null default (for examplehostnameandinitial_delivery_count) stayNone.decode_framehandles the AMQPlist0(0x45) body encoding. A performative with all fields omitted can arrive aslist0, which carries no count byte. The previous code readdata[5]out of range and raisedIndexError. The decoder now treatslist0as zero fields and pads to the full field count._PERFORMATIVE_FIELD_DEFAULTScomprehension carriespylint: disable=protected-access,no-memberand a mypytype: ignorefor its access to_codeand_definition. These attributes are set on the performative classes at import time, so static analysis cannot see them. This matches the existing suppressions in_encode.py._pyamqpcopies. The two copies stay byte-identical, and a test guards against drift.max_frame_sizewith a later field set, a short Transfer with a trailing payload, the performatives without namedtuple defaults, thelist32body encoding, short SASL frames, thelist0End and Close encodings, and the per-performative field counts.api.mdandapi.metadata.ymlfor azure-eventhub are added, generated withazpysdk apistub(parser 0.3.28), to satisfy the API consistency check.Test plan
pytest tests/pyamqp_tests/unittest/test_decode.py, 32 passed.pytest tests/unittests/test_pyamqp_decode.py, 25 passed.pylinton_decode.pyreports zero errors with the CI plugin versions (pylint 4.0.4, azure-pylint-guidelines-checker 0.5.7).